Skip to content

feat: add 404 page and serve metrics on separate port#35

Merged
rsbh merged 12 commits intomainfrom
fix_404_otel_server
Apr 15, 2026
Merged

feat: add 404 page and serve metrics on separate port#35
rsbh merged 12 commits intomainfrom
fix_404_otel_server

Conversation

@rsbh
Copy link
Copy Markdown
Member

@rsbh rsbh commented Apr 14, 2026

Summary

  • Handle /api/page errors in page context with errorStatus state instead of silently catching
  • Show custom 404 page using Apsara EmptyState component when page not found (SSR + client-side)
  • Move Prometheus metrics from /api/metrics route to separate port (default 9090, configurable via telemetry.port)

Test plan

  • Navigate to non-existent page → 404 page renders centered
  • Valid pages still render correctly
  • SSR returns HTTP 404 status for missing pages
  • Client-side navigation to missing page shows 404
  • Metrics available on port 9090 (curl localhost:9090/metrics)
  • Old /api/metrics route returns 404

🤖 Generated with Claude Code

rsbh and others added 6 commits April 14, 2026 15:31
Handle non-ok responses from /api/page instead of silently catching.
Track errorStatus as HTTP status code (number | null) to distinguish
error states from loading state.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Wire existing NotFound component into DocsPage using errorStatus
from page context.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add getInitialErrorStatus() to compute error state from initialPage
and pathname, so SSR also shows NotFound component on missing pages.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace manual Flex/Headline/Text layout with Apsara EmptyState
component for consistent empty state styling.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PrometheusExporter now starts its own HTTP server on configurable
port (default 9090). Removes /api/metrics route and mock request
handler. Adds telemetry.port to config schema.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 14, 2026

📝 Walkthrough

Summary by CodeRabbit

  • New Features

    • Telemetry can be enabled with configurable port (default 9090); Prometheus metrics exposed via OpenTelemetry
    • Improved 404 handling and explicit "Page not found" display
  • Documentation

    • Added telemetry configuration section and examples
    • Added example docs config and script to run docs server
  • Refactor

    • Telemetry implementation reworked to use server hooks and inline OpenTelemetry setup; legacy telemetry APIs/endpoints removed

Walkthrough

Adds errorStatus tracking to page context and DocsPage to surface NotFound states; refactors NotFound UI to use EmptyState; removes the standalone telemetry module and /api/metrics endpoint; inlines OpenTelemetry setup in the Nitro telemetry plugin, adds a chronicle:ssr-rendered Nitro hook for SSR timing, and exposes telemetry port/config and docs updates.

Changes

Cohort / File(s) Summary
Page context & Docs rendering
packages/chronicle/src/lib/page-context.tsx, packages/chronicle/src/pages/DocsPage.tsx
Added `errorStatus: number
NotFound UI
packages/chronicle/src/pages/NotFound.tsx, packages/chronicle/src/pages/NotFound.module.css
Replaced custom layout with EmptyState component and added .emptyState CSS class for centering.
Telemetry: plugin & SSR hook
packages/chronicle/src/server/plugins/telemetry.ts, packages/chronicle/src/server/entry-server.tsx
Removed dependency on local telemetry module; inlined OpenTelemetry/Prometheus exporter and MeterProvider in Nitro plugin; created request and duration instruments, record metrics in request/response hooks, added chronicle:ssr-rendered Nitro hook and registered SSR-timing handler; call hook from SSR entry-server; shutdown provider/exporter on Nitro close.
Telemetry: removed module & API
packages/chronicle/src/server/telemetry.ts (deleted), packages/chronicle/src/server/api/metrics.ts (deleted)
Deleted standalone telemetry API surface and the /api/metrics endpoint previously exposing metrics.
Configuration & docs
packages/chronicle/src/types/config.ts, docs/chronicle.yaml, docs/configuration.mdx, package.json
Added telemetry.port (1–65535, default 9090) to config schema; enabled telemetry in docs chronicle.yaml; updated docs to document telemetry fields and metrics endpoint; added start:docs npm script pointing to docs/chronicle.yaml.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant NitroPlugin as Nitro Telemetry Plugin
    participant Exporter as OpenTelemetry/Exporter
    participant Entry as entry-server (SSR)

    Client->>NitroPlugin: HTTP request (method, path)
    NitroPlugin-->>NitroPlugin: store start time on event.context._requestStart (request hook)
    NitroPlugin->>Exporter: increment request counter (on response)
    NitroPlugin->>Exporter: record request duration (on response)

    Client->>Entry: SSR render (if SSR route)
    Entry->>NitroPlugin: callHook('chronicle:ssr-rendered', route, status, duration)
    NitroPlugin->>Exporter: record SSR render duration (with route,status)

    NitroPlugin->>Exporter: shutdown (on Nitro close)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • rohilsurana
  • rohanchkrabrty
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the two main changes: adding a 404 page and moving metrics to a separate port, which are the core objectives of this PR.
Description check ✅ Passed The description clearly outlines the three key changes (404 page handling, error state exposure, metrics on separate port) and provides a concrete test plan related to the changeset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix_404_otel_server

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/chronicle/src/lib/page-context.tsx`:
- Line 57: The conditional using pathname.startsWith('/apis') is too broad and
matches routes like '/apis-foo'; update the check that currently reads something
like if (pathname === '/' || pathname.startsWith('/apis')) to only treat the
top-level apis route by changing it to check for pathname === '/apis' or
pathname.startsWith('/apis/') (i.e., if (pathname === '/' || pathname ===
'/apis' || pathname.startsWith('/apis/')) ), and make the same change for the
other occurrence that uses pathname.startsWith('/apis').

In `@packages/chronicle/src/pages/DocsPage.tsx`:
- Around line 13-14: The current DocsPage returns null for any errorStatus that
isn't 404, causing a blank screen on server errors; update the DocsPage
component's error handling (the block checking errorStatus and page) so that
when errorStatus is set and not 404 it renders a proper error/failure UI instead
of null—e.g., render an Error or ServerError component (or a generic error
message) for non-404 statuses, keeping the existing NotFound render for 404 and
preserving the existing page render when page is present.

In `@packages/chronicle/src/types/config.ts`:
- Line 73: The telemetry.port schema currently uses
z.number().optional().default(9090) and allows invalid TCP port values
(negatives, >65535, non-integers); update the schema for telemetry.port to
enforce an integer in the valid TCP range by adding integer, min and max
constraints (e.g., use
z.number().int().min(1).max(65535).optional().default(9090)) so only valid ports
are accepted while preserving the default.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 4be53968-c75d-415d-9f2e-4a2bab8b2088

📥 Commits

Reviewing files that changed from the base of the PR and between 752b303 and ef09ec4.

📒 Files selected for processing (8)
  • packages/chronicle/src/lib/page-context.tsx
  • packages/chronicle/src/pages/DocsPage.tsx
  • packages/chronicle/src/pages/NotFound.module.css
  • packages/chronicle/src/pages/NotFound.tsx
  • packages/chronicle/src/server/api/metrics.ts
  • packages/chronicle/src/server/plugins/telemetry.ts
  • packages/chronicle/src/server/telemetry.ts
  • packages/chronicle/src/types/config.ts
💤 Files with no reviewable changes (2)
  • packages/chronicle/src/server/plugins/telemetry.ts
  • packages/chronicle/src/server/api/metrics.ts

Comment thread packages/chronicle/src/lib/page-context.tsx Outdated
Comment thread packages/chronicle/src/pages/DocsPage.tsx
Comment thread packages/chronicle/src/types/config.ts Outdated
rsbh and others added 5 commits April 14, 2026 16:06
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Default imports fail in ESM — these packages only export named.
Fixes build failure.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Tighten /apis route matching to avoid false positives on /apis-foo
- Handle non-404 errors in DocsPage to avoid blank screen
- Validate telemetry.port as int in 1-65535 range

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move OTel setup (MeterProvider, PrometheusExporter, counters/histograms)
into plugins/telemetry.ts. Add custom 'chronicle:ssr-rendered' Nitro
hook so entry-server can record SSR metrics via useNitroApp().hooks.

Removes separate telemetry.ts module — all state lives in plugin scope.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/configuration.mdx`:
- Around line 286-290: Update the docs table to document the validated
constraint for telemetry.port: change the Description for `port`
(telemetry.port) to indicate it must be an integer in the range 1–65535 (e.g.,
"Port for Prometheus metrics endpoint; integer 1-65535") so it matches the
runtime validation and avoids startup errors when users copy the example.

In `@packages/chronicle/src/server/entry-server.tsx`:
- Line 101: The call to useNitroApp().hooks.callHook('chronicle:ssr-rendered',
pathname, status, renderDuration) returns a Promise and must be handled to avoid
unhandled rejections; update the code in the entry-server SSR path to either
await the call (e.g., await useNitroApp().hooks.callHook(...)) or explicitly
detach it with void useNitroApp().hooks.callHook(...).catch(err => /* log or
ignore */) so errors are caught; refer to useNitroApp, hooks.callHook and the
'chronicle:ssr-rendered' invocation with pathname, status, renderDuration to
locate and fix the line.

In `@packages/chronicle/src/server/plugins/telemetry.ts`:
- Around line 38-40: The shutdown hook currently calls both provider.shutdown()
and exporter.shutdown(); remove the redundant exporter.shutdown() call so that
only provider.shutdown() is invoked in the nitroApp.hooks.hook('close', ...)
callback, since the MeterProvider (provider) will already shut down all
registered readers including the exporter.
- Around line 23-25: The PrometheusExporter is started unguarded (const exporter
= new PrometheusExporter({ port })) which will spawn an HTTP listener and break
edge/serverless runtimes; update the telemetry init in this plugin to only
initialize/exporter when running in a node-server preset or, alternatively,
construct PrometheusExporter with preventServerStart: true and skip starting its
server so metrics can be mounted into the existing HTTP server. Concretely:
check the runtime/preset or config (e.g., config.telemetry and deployment/preset
check) before creating PrometheusExporter, or instantiate new
PrometheusExporter({ port, preventServerStart: true }) and document that manual
integration into the HTTP server is required; ensure
MeterProvider/resource/readers setup only proceeds after that guard.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 676d47ad-0996-41cd-af58-ab81d06f493c

📥 Commits

Reviewing files that changed from the base of the PR and between ef09ec4 and adb4046.

📒 Files selected for processing (9)
  • docs/chronicle.yaml
  • docs/configuration.mdx
  • package.json
  • packages/chronicle/src/lib/page-context.tsx
  • packages/chronicle/src/pages/DocsPage.tsx
  • packages/chronicle/src/server/entry-server.tsx
  • packages/chronicle/src/server/plugins/telemetry.ts
  • packages/chronicle/src/server/telemetry.ts
  • packages/chronicle/src/types/config.ts
💤 Files with no reviewable changes (1)
  • packages/chronicle/src/server/telemetry.ts
✅ Files skipped from review due to trivial changes (2)
  • docs/chronicle.yaml
  • package.json
🚧 Files skipped from review as they are similar to previous changes (3)
  • packages/chronicle/src/types/config.ts
  • packages/chronicle/src/pages/DocsPage.tsx
  • packages/chronicle/src/lib/page-context.tsx

Comment thread docs/configuration.mdx
Comment on lines +286 to +290
| Field | Type | Description | Default |
|-------|------|-------------|---------|
| `enabled` | `boolean` | Enable/disable telemetry | `false` |
| `serviceName` | `string` | OpenTelemetry service name | `chronicle` |
| `port` | `number` | Port for Prometheus metrics endpoint | `9090` |
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Document the validated telemetry.port constraint.

The table says number, but the implementation validates an integer in 1-65535. Please mirror that here so users do not hit startup validation errors after copying the example.

Proposed fix
-| `port` | `number` | Port for Prometheus metrics endpoint | `9090` |
+| `port` | `integer` | Port for Prometheus metrics endpoint. Must be in `1-65535`. | `9090` |
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
| Field | Type | Description | Default |
|-------|------|-------------|---------|
| `enabled` | `boolean` | Enable/disable telemetry | `false` |
| `serviceName` | `string` | OpenTelemetry service name | `chronicle` |
| `port` | `number` | Port for Prometheus metrics endpoint | `9090` |
| Field | Type | Description | Default |
|-------|------|-------------|---------|
| `enabled` | `boolean` | Enable/disable telemetry | `false` |
| `serviceName` | `string` | OpenTelemetry service name | `chronicle` |
| `port` | `integer` | Port for Prometheus metrics endpoint. Must be in `1-65535`. | `9090` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/configuration.mdx` around lines 286 - 290, Update the docs table to
document the validated constraint for telemetry.port: change the Description for
`port` (telemetry.port) to indicate it must be an integer in the range 1–65535
(e.g., "Port for Prometheus metrics endpoint; integer 1-65535") so it matches
the runtime validation and avoids startup errors when users copy the example.

const status = !page && !isApiRoute && slug.length > 0 ? 404 : 200;

recordSSRRender(pathname, status, renderDuration);
useNitroApp().hooks.callHook('chronicle:ssr-rendered', pathname, status, renderDuration);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

In Nitro/Hookable, does hooks.callHook(...) return a Promise that should be awaited or otherwise handled to avoid unhandled rejections?

💡 Result:

Yes, in Nitro/Hookable, hooks.callHook(...) returns a Promise that should be awaited or otherwise handled to avoid unhandled rejections if any hook callbacks reject.

Citations:


🏁 Script executed:

# Find and examine the entry-server.tsx file
fd -t f entry-server.tsx

Repository: raystack/chronicle

Length of output: 108


🏁 Script executed:

# Read the file around line 101 to understand context
head -n 120 packages/chronicle/src/server/entry-server.tsx | tail -n 40

Repository: raystack/chronicle

Length of output: 1369


🏁 Script executed:

# Get more context - full function containing line 101
sed -n '80,120p' packages/chronicle/src/server/entry-server.tsx

Repository: raystack/chronicle

Length of output: 929


🏁 Script executed:

# Get the full function signature and start
sed -n '50,110p' packages/chronicle/src/server/entry-server.tsx

Repository: raystack/chronicle

Length of output: 2003


🏁 Script executed:

# Check if the handler function is async
sed -n '40,105p' packages/chronicle/src/server/entry-server.tsx

Repository: raystack/chronicle

Length of output: 2306


🏁 Script executed:

# Get the complete handler function definition from start to line 110
sed -n '1,110p' packages/chronicle/src/server/entry-server.tsx | head -n 50

Repository: raystack/chronicle

Length of output: 1897


🏁 Script executed:

# Search for the handler function signature
rg -A 80 "export default" packages/chronicle/src/server/entry-server.tsx | head -n 100

Repository: raystack/chronicle

Length of output: 2745


🏁 Script executed:

# Check the telemetry hook registration to understand if it's meant to be fire-and-forget
cat packages/chronicle/src/server/plugins/telemetry.ts

Repository: raystack/chronicle

Length of output: 2247


🏁 Script executed:

# Look for other usages of callHook in the codebase to see if there's a pattern
rg "callHook" packages/chronicle/src/server/

Repository: raystack/chronicle

Length of output: 202


Handle the Nitro hook promise explicitly.

callHook(...) is async and returns a Promise. Since the fetch handler is async, either await it here or intentionally detach it with void ... .catch(...) to avoid unhandled promise rejection.

Proposed fix
-    useNitroApp().hooks.callHook('chronicle:ssr-rendered', pathname, status, renderDuration);
+    await useNitroApp().hooks.callHook('chronicle:ssr-rendered', pathname, status, renderDuration);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
useNitroApp().hooks.callHook('chronicle:ssr-rendered', pathname, status, renderDuration);
await useNitroApp().hooks.callHook('chronicle:ssr-rendered', pathname, status, renderDuration);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/chronicle/src/server/entry-server.tsx` at line 101, The call to
useNitroApp().hooks.callHook('chronicle:ssr-rendered', pathname, status,
renderDuration) returns a Promise and must be handled to avoid unhandled
rejections; update the code in the entry-server SSR path to either await the
call (e.g., await useNitroApp().hooks.callHook(...)) or explicitly detach it
with void useNitroApp().hooks.callHook(...).catch(err => /* log or ignore */) so
errors are caught; refer to useNitroApp, hooks.callHook and the
'chronicle:ssr-rendered' invocation with pathname, status, renderDuration to
locate and fix the line.

Comment on lines +23 to +25
const port = config.telemetry?.port ?? 9090
const exporter = new PrometheusExporter({ port })
const provider = new MeterProvider({ resource, readers: [exporter] })
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

Does @opentelemetry/exporter-prometheus start a standalone Node.js HTTP server, and is that supported in edge/serverless runtimes like Cloudflare Workers or Vercel?

💡 Result:

Yes, @opentelemetry/exporter-prometheus starts a standalone Node.js HTTP server by default on port 9464 to expose metrics for Prometheus scraping, as shown in official documentation and examples where instantiation with { port: 9464 } enables access at http://localhost:9464/metrics. This can be prevented using the preventServerStart: true option, allowing manual control via exporter.getMetricsRequestHandler(req, res) to integrate into an existing server. However, it is not supported in edge/serverless runtimes like Cloudflare Workers or Vercel Edge Functions. Cloudflare Workers (V8 isolates) fail to import due to Node.js "http" module dependencies, even with preventServerStart: true and node_compat=true (errors: "Could not resolve 'http'", "globalThis.XMLHttpRequest is not a constructor"). Vercel documentation focuses on tracing (OTLP), with no Prometheus exporter mention; Edge runtime custom spans unsupported. Use OTLP/HTTP exporters or platform-native observability instead.

Citations:


🏁 Script executed:

cat -n packages/chronicle/src/server/plugins/telemetry.ts | head -50

Repository: raystack/chronicle

Length of output: 2230


🏁 Script executed:

rg -i "preset|vercel|cloudflare|node-server" packages/chronicle --type ts --type js | head -20

Repository: raystack/chronicle

Length of output: 1920


🏁 Script executed:

cat -n packages/chronicle/src/lib/config.ts | head -100

Repository: raystack/chronicle

Length of output: 1222


🏁 Script executed:

cat -n packages/chronicle/src/types/config.ts

Repository: raystack/chronicle

Length of output: 3802


Guard this exporter to supported runtimes or gate telemetry to node-server deployments.

PrometheusExporter starts a standalone HTTP listener by default. This works for long-lived node-server deployments but will fail in edge/serverless environments like Vercel or Cloudflare Workers, which cannot expose arbitrary ports. The plugin currently has no runtime detection to prevent initialization on incompatible presets. Either:

  • Gate the plugin to node-server preset only, or
  • Use preventServerStart: true and manually integrate metrics into the existing HTTP server, or
  • Document that telemetry is only supported for node-server deployment

Without a guard, enabling telemetry.enabled: true in config will cause deployments to fail on unsupported runtimes.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/chronicle/src/server/plugins/telemetry.ts` around lines 23 - 25, The
PrometheusExporter is started unguarded (const exporter = new
PrometheusExporter({ port })) which will spawn an HTTP listener and break
edge/serverless runtimes; update the telemetry init in this plugin to only
initialize/exporter when running in a node-server preset or, alternatively,
construct PrometheusExporter with preventServerStart: true and skip starting its
server so metrics can be mounted into the existing HTTP server. Concretely:
check the runtime/preset or config (e.g., config.telemetry and deployment/preset
check) before creating PrometheusExporter, or instantiate new
PrometheusExporter({ port, preventServerStart: true }) and document that manual
integration into the HTTP server is required; ensure
MeterProvider/resource/readers setup only proceeds after that guard.

Comment on lines +38 to +40
nitroApp.hooks.hook('close', async () => {
await provider.shutdown()
await exporter.shutdown()
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

In OpenTelemetry JS, does MeterProvider.shutdown()already shut down registered readers such asPrometheusExporter, making an extra exporter.shutdown() redundant?

💡 Result:

Yes, MeterProvider.shutdown shuts down registered readers such as PrometheusExporter, making an extra exporter.shutdown redundant.

Citations:


🏁 Script executed:

cat -n packages/chronicle/src/server/plugins/telemetry.ts

Repository: raystack/chronicle

Length of output: 2646


Remove the redundant exporter.shutdown() call.

The exporter is registered as a reader when the MeterProvider is created on line 25. When provider.shutdown() is called, it automatically shuts down all registered readers, including the exporter. Calling exporter.shutdown() again is redundant and can cause issues during process shutdown.

Proposed fix
  nitroApp.hooks.hook('close', async () => {
    await provider.shutdown()
-   await exporter.shutdown()
  })
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/chronicle/src/server/plugins/telemetry.ts` around lines 38 - 40, The
shutdown hook currently calls both provider.shutdown() and exporter.shutdown();
remove the redundant exporter.shutdown() call so that only provider.shutdown()
is invoked in the nitroApp.hooks.hook('close', ...) callback, since the
MeterProvider (provider) will already shut down all registered readers including
the exporter.

NitroRuntimeHooks types event as HTTPEvent (only has req), but H3
runtime passes H3Event which has context, url, etc. Cast safely
since runtime is always H3Event.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/chronicle/src/server/plugins/telemetry.ts`:
- Around line 52-60: In the response hook in telemetry.ts (the
nitroApp.hooks.hook('response', ...)), stop using new
URL(event.req.url).pathname because event.req.url can be a relative path and
will throw; instead derive the route from the H3 event directly (use event.path)
or, if you must parse, construct a URL with a base (e.g., new URL(event.req.url
?? '/', 'http://localhost').pathname) before calling .pathname; update the code
that sets the route variable and keep the rest of the
requestCounter/requestDuration calls unchanged.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c80c0089-9ae6-4809-bdf6-051943619a0e

📥 Commits

Reviewing files that changed from the base of the PR and between adb4046 and e422e7c.

📒 Files selected for processing (1)
  • packages/chronicle/src/server/plugins/telemetry.ts

Comment on lines 52 to 60
nitroApp.hooks.hook('response', (res, event) => {
if (!event.context._requestStart) return
const duration = performance.now() - event.context._requestStart
recordRequest(event.method, event.path, res.status, duration)
const start = (event as H3Event).context._requestStart as number | undefined
if (start === undefined) return
const duration = performance.now() - start
const method = event.req.method
const route = new URL(event.req.url).pathname
requestCounter.add(1, { method, route, status: res.status })
requestDuration.record(duration, { method, route, status: res.status })
})
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify that event.req.url in Nitro/H3 is a relative path
# Search for how other Nitro plugins or H3 middleware handle URL parsing

rg -n -C3 'event\.req\.url|getRequestURL' --type ts packages/chronicle/src/server

Repository: raystack/chronicle

Length of output: 1387


🏁 Script executed:

#!/bin/bash
# Search for usage of event.path or other URL handling patterns in H3/Nitro plugins
rg -n 'event\.path|getRequestURL|event\.node\.req' --type ts packages/chronicle/src/server/plugins

Repository: raystack/chronicle

Length of output: 44


🏁 Script executed:

#!/bin/bash
# Check what properties are available on H3Event by looking at imports and type usage
rg -n 'H3Event|defineEventHandler|import.*h3' --type ts packages/chronicle/src/server | head -40

Repository: raystack/chronicle

Length of output: 402


🌐 Web query:

H3 H3Event event.req.url relative path absolute pathname

💡 Result:

In H3 (the high-performance HTTP framework used by Nuxt/Nitro), an H3Event object is created for each incoming HTTP request. The event.req property provides access to the incoming HTTP request information, modeled after the native Web API Request with runtime addons. event.req.url is a string containing the absolute URL of the request (full URL including protocol, host, port, pathname, search, and hash if present). This matches the standard behavior of the Fetch API's Request.url property, which always returns the absolute URL. However, in server environments like Node.js, H3 constructs event.req such that event.req.url typically holds the relative path (pathname + search) from the request URL, without the origin (e.g., "/api/users?id=123" for a request to "https://example.com/api/users?id=123"). This is consistent with traditional Node.js http.IncomingMessage.url and common server framework conventions, allowing logging like [GET] /api/users. To get the relative path (just pathname + search): - Use event.req.url directly (e.g., "/api/users?id=123") - Or prefer event.path, which is explicitly the request path (same value, recommended over parsing event.req.url) To get the absolute pathname (full URL as string): - Use getRequestURL(event) utility (e.g., "https://example.com/api/users?id=123") Examples from official docs: app.get("/", async (event) => { console.log(event.req.url); // "/" const fullUrl = getRequestURL(event); // "https://example.com/" }); event.path // "/test?test=123" (relative path example) event.url provides a parsed URL object for the full request URL. Sources confirm event.req is Web Request-based but adapted for server use, with event.path preferred for relative paths post-refactors.

Citations:


🏁 Script executed:

#!/bin/bash
# Check if there are tests for telemetry plugin
fd -e test.ts -e spec.ts -e test.js packages/chronicle | grep -i telemetry

# Also check test directories
find packages/chronicle -type f -name "*test*" -o -name "*spec*" | grep -i telemetry

Repository: raystack/chronicle

Length of output: 439


🏁 Script executed:

#!/bin/bash
# Check if event.path is used elsewhere in the codebase for URL parsing
rg -n 'event\.path' --type ts packages/chronicle/src/server

Repository: raystack/chronicle

Length of output: 185


new URL(event.req.url) will throw for relative paths and crash telemetry.

In H3/Nitro, event.req.url is a relative path (e.g., /api/users?query=1), not an absolute URL. Calling new URL('/api/users') without a base throws TypeError: Invalid URL, which will crash every response and disable telemetry entirely.

Use event.path instead, which H3 provides specifically for this purpose:

   nitroApp.hooks.hook('response', (res, event) => {
     const start = (event as H3Event).context._requestStart as number | undefined
     if (start === undefined) return
     const duration = performance.now() - start
     const method = event.req.method
-    const route = new URL(event.req.url).pathname
+    const route = event.path.split('?')[0]
     requestCounter.add(1, { method, route, status: res.status })
     requestDuration.record(duration, { method, route, status: res.status })
   })

Alternatively, if you need to construct a URL object, use a base: new URL(event.req.url ?? '/', 'http://localhost').pathname.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
nitroApp.hooks.hook('response', (res, event) => {
if (!event.context._requestStart) return
const duration = performance.now() - event.context._requestStart
recordRequest(event.method, event.path, res.status, duration)
const start = (event as H3Event).context._requestStart as number | undefined
if (start === undefined) return
const duration = performance.now() - start
const method = event.req.method
const route = new URL(event.req.url).pathname
requestCounter.add(1, { method, route, status: res.status })
requestDuration.record(duration, { method, route, status: res.status })
})
nitroApp.hooks.hook('response', (res, event) => {
const start = (event as H3Event).context._requestStart as number | undefined
if (start === undefined) return
const duration = performance.now() - start
const method = event.req.method
const route = event.path.split('?')[0]
requestCounter.add(1, { method, route, status: res.status })
requestDuration.record(duration, { method, route, status: res.status })
})
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/chronicle/src/server/plugins/telemetry.ts` around lines 52 - 60, In
the response hook in telemetry.ts (the nitroApp.hooks.hook('response', ...)),
stop using new URL(event.req.url).pathname because event.req.url can be a
relative path and will throw; instead derive the route from the H3 event
directly (use event.path) or, if you must parse, construct a URL with a base
(e.g., new URL(event.req.url ?? '/', 'http://localhost').pathname) before
calling .pathname; update the code that sets the route variable and keep the
rest of the requestCounter/requestDuration calls unchanged.

@rsbh rsbh merged commit d35822e into main Apr 15, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants